Skip to content

Save replay tests to test dir#317

Merged
aseembits93 merged 5 commits into
mainfrom
replay-test-save-dir
Jun 12, 2025
Merged

Save replay tests to test dir#317
aseembits93 merged 5 commits into
mainfrom
replay-test-save-dir

Conversation

@aseembits93
Copy link
Copy Markdown
Contributor

@aseembits93 aseembits93 commented Jun 11, 2025 •
edited by github-actions Bot
Loading

PR Type

Bug fix, Enhancement

Description

  • Relax --benchmarks-root subdirectory requirement

  • Create replay tests under tests_root directory

Changes walkthrough 📝

Relevant files
Error handling
cli.py
Remove benchmarks_root subdir assertion                                   

codeflash/cli_cmds/cli.py

  • Removed assertion enforcing benchmarks_root under tests_root
  • No longer checks subdirectory relationship between roots
    		• +0/-3     
    Bug fix
    optimizer.py
    Save replay tests under tests_root                                             

    codeflash/optimization/optimizer.py

  • Changed tempfile.mkdtemp directory to use tests_root
  • Ensures replay tests saved in tests directory, not benchmarks
    		• +1/-1     
    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
    •

    @aseembits93 aseembits93 marked this pull request as draft June 11, 2025 23:43
    @github-actions
    Copy link
    Copy Markdown
    Contributor

    github-actions Bot commented Jun 11, 2025 •
    edited
    Loading

    PR Reviewer Guide 🔍

    (Review updated until commit b6a68ed)

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Path validation relaxed

    The removal of the check enforcing benchmarks_root as a subdirectory of tests_root could lead to unexpected file path handling. Ensure downstream operations correctly support benchmark roots outside the tests directory.

     
    assert Path(args.module_root).is_dir(), f"--module-root {args.module_root} must be a valid directory"
assert args.tests_root is not None, "--tests-root must be specified"
assert Path(args.tests_root).is_dir(), f"--tests-root {args.tests_root} must be a valid directory"
if args.benchmark:
    assert args.benchmarks_root is not None, "--benchmarks-root must be specified when running with --benchmark"
    assert Path(args.benchmarks_root).is_dir(), (
        f"--benchmarks-root {args.benchmarks_root} must be a valid directory"
    )
    Temporary directory cleanup

    tempfile.mkdtemp creates a new replay tests directory under tests_root but no cleanup mechanism is shown. Confirm whether these directories should be removed after use to avoid resource leaks.

     
    self.replay_tests_dir = Path(
    tempfile.mkdtemp(prefix="codeflash_replay_tests_", dir=self.args.tests_root)
)

    @github-actions
    Copy link
    Copy Markdown
    Contributor

    github-actions Bot commented Jun 11, 2025 •
    edited
    Loading

    PR Code Suggestions ✨

    Latest suggestions up to b6a68ed
    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Use TemporaryDirectory for cleanup

    Consider using TemporaryDirectory to manage the lifecycle of the replay tests folder
    automatically, ensuring it is cleaned up when no longer needed.

    codeflash/optimization/optimizer.py [130-132]

    -self.replay_tests_dir = Path(
-    tempfile.mkdtemp(prefix="codeflash_replay_tests_", dir=self.args.tests_root)
+self._tempdir = tempfile.TemporaryDirectory(
+    prefix="codeflash_replay_tests_", dir=self.args.tests_root
 )
+self.replay_tests_dir = Path(self._tempdir.name)
    Suggestion importance[1-10]: 7

    __

    Why: Switching to TemporaryDirectory ensures automatic cleanup of the replay directory, improving resource management without altering core logic.

    		Medium

    Previous suggestions

    Suggestions up to commit b6a68ed
    CategorySuggestion                                                                                                                                    Impact
    Security
    Revalidate benchmarks_root location

    Reinstate the check ensuring that benchmarks_root is within tests_root to prevent
    misconfiguration and unexpected file writes. This safeguards that benchmarks run
    only inside the designated tests directory.

    codeflash/cli_cmds/cli.py [135-140]

     if args.benchmark:
     assert args.benchmarks_root is not None, "--benchmarks-root must be specified when running with --benchmark"
     assert Path(args.benchmarks_root).is_dir(), (
         f"--benchmarks-root {args.benchmarks_root} must be a valid directory"
     )
+    assert Path(args.benchmarks_root).resolve().is_relative_to(Path(args.tests_root).resolve()), (
+        f"--benchmarks-root {args.benchmarks_root} must be a subdirectory of --tests-root {args.tests_root}"
+    )
     if env_utils.get_pr_number() is not None:
    Suggestion importance[1-10]: 7

    __

    Why: The reinstated is_relative_to assertion correctly enforces that benchmarks_root is a subdirectory of tests_root, preventing misconfiguration and unintended file writes.

    		Medium
    General
    Resolve tests_root before mkdtemp

    Resolve tests_root to an absolute path before passing it to mkdtemp to avoid
    creating directories in unexpected locations when relative paths are used.

    codeflash/optimization/optimizer.py [130-132]

    +base_tests_root = Path(self.args.tests_root).resolve()
 self.replay_tests_dir = Path(
-    tempfile.mkdtemp(prefix="codeflash_replay_tests_", dir=self.args.tests_root)
+    tempfile.mkdtemp(prefix="codeflash_replay_tests_", dir=str(base_tests_root))
 )
    Suggestion importance[1-10]: 6

    __

    Why: Resolving tests_root ensures mkdtemp creates the replay directory at the intended absolute path, avoiding unexpected directory locations.

    		Low

    @aseembits93
    Copy link
    Copy Markdown
    Contributor Author

    aseembits93 commented Jun 11, 2025

    Screenshot 2025-06-11 at 4 48 40 PM

    @aseembits93 aseembits93 marked this pull request as ready for review June 11, 2025 23:55
    @github-actions
    Copy link
    Copy Markdown
    Contributor

    github-actions Bot commented Jun 11, 2025

    Persistent review updated to latest commit b6a68ed

    KRRT7
    KRRT7 suggested changes Jun 12, 2025
    View reviewed changes
    Comment thread codeflash/optimization/optimizer.py
    Comment thread docs/docs/optimizing-with-codeflash/benchmarking.md Outdated
    @aseembits93 aseembits93 requested a review from KRRT7 June 12, 2025 00:38
    @aseembits93 aseembits93 merged commit e10630d into main Jun 12, 2025
    16 checks passed
    @aseembits93 aseembits93 deleted the replay-test-save-dir branch June 12, 2025 00:46
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    @misrasaurabh1 misrasaurabh1 Awaiting requested review from misrasaurabh1 misrasaurabh1 is a code owner

    1 more reviewer

    @KRRT7 KRRT7 KRRT7 approved these changes

    Reviewers whose approvals may not affect merge requirements

    Assignees

    No one assigned

    Labels

    Review effort 2/5

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    2 participants

    @aseembits93 @KRRT7